Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

examples: Interpolation tutorial notebook #2252

Merged
merged 3 commits into from
Apr 4, 2024
Merged

examples: Interpolation tutorial notebook #2252

merged 3 commits into from
Apr 4, 2024

Conversation

mloubout
Copy link
Contributor

No description provided.

@mloubout mloubout added the examples examples label Oct 30, 2023
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.41%. Comparing base (364997c) to head (bc7bb4a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2252   +/-   ##
=======================================
  Coverage   79.41%   79.41%           
=======================================
  Files         232      232           
  Lines       43578    43578           
  Branches     8072     8072           
=======================================
  Hits        34606    34606           
  Misses       8215     8215           
  Partials      757      757           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -432,15 +432,7 @@
"cell_type": "code",
Copy link
Contributor

@EdCaunt EdCaunt Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover output in this cell probably doesn't want to be there


Reply via ReviewNB

@@ -0,0 +1,617 @@
{
Copy link
Contributor

@georgebisbas georgebisbas Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall needs a lot comments to be more "educational", then we could have another iteration

  • injection that inject values at one or more given sparse positions (i.e point sources)
  • interpolation that reads the value of a field at one or more given sparse positions (i.e point measurments)

Reply via ReviewNB

@@ -0,0 +1,617 @@
{
Copy link
Contributor

@georgebisbas georgebisbas Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid star imports?


Reply via ReviewNB

@@ -0,0 +1,617 @@
{
Copy link
Contributor

@georgebisbas georgebisbas Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment like: Some variable declarations

introduce diff between f and u (what is save)


Reply via ReviewNB

@@ -0,0 +1,617 @@
{
Copy link
Contributor

@georgebisbas georgebisbas Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are nice figs, I would really like to had in the IPDPS paper


Reply via ReviewNB

Copy link
Contributor

@georgebisbas georgebisbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much needed PR, added some comments

@@ -0,0 +1,630 @@
{
Copy link
Contributor

@EdCaunt EdCaunt Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #13.    ax.legend()

Missing plt.show()


Reply via ReviewNB

@@ -0,0 +1,630 @@
{
Copy link
Contributor

@EdCaunt EdCaunt Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #2.    ax.imshow(u.data[1], vmin=0, vmax=1, cmap="jet", extent=[0,1,0,1])

Missing plt.show()


Reply via ReviewNB

@@ -0,0 +1,630 @@
{
Copy link
Contributor

@EdCaunt EdCaunt Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #2.    ax.imshow(u.data[1], vmin=0, vmax=2, cmap="jet", extent=[0,1,0,1])

Missing a plt.show()


Reply via ReviewNB

@mloubout mloubout force-pushed the notebook-fix branch 5 times, most recently from b5c8c46 to 0c48414 Compare December 15, 2023 14:05
@mloubout mloubout changed the title examples: fix subdomain notebook examples: Interpolation tutorial notebook Apr 4, 2024
@@ -0,0 +1,965 @@
{
Copy link
Contributor

@EdCaunt EdCaunt Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sparse "positions"


Reply via ReviewNB

@@ -0,0 +1,965 @@
{
Copy link
Contributor

@EdCaunt EdCaunt Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #3.    coords = np.random.rand(npoint, 2)/2 + .25

Do you need to set a seed here? Otherwise this is going to be non-deterministic


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's intentional. This way, you can see different scenarios (overlap, ...) when user runs it

@@ -0,0 +1,965 @@
{
Copy link
Contributor

@EdCaunt EdCaunt Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"a Devito object, representing a"

"and the data at those positions"

"is the dimensionality of the Grid"

Remove the first "at the sparse positions"


Reply via ReviewNB

@@ -0,0 +1,965 @@
{
Copy link
Contributor

@EdCaunt EdCaunt Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Multilinear" perhaps?


Reply via ReviewNB

@@ -0,0 +1,965 @@
{
Copy link
Contributor

@EdCaunt EdCaunt Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #14.    ax.legend()

Needs a plt.show()


Reply via ReviewNB

@@ -0,0 +1,965 @@
{
Copy link
Contributor

@EdCaunt EdCaunt Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #2.    op()

The repeated cells with op() could use some kind of explanation


Reply via ReviewNB

@@ -0,0 +1,965 @@
{
Copy link
Contributor

@EdCaunt EdCaunt Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #3.    ax.imshow(u.data[1], vmin=0, vmax=1, cmap="jet", extent=[0,1,0,1])

A colourbar is needed here


Reply via ReviewNB

@@ -0,0 +1,965 @@
{
Copy link
Contributor

@EdCaunt EdCaunt Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Colourbar


Reply via ReviewNB

@@ -0,0 +1,965 @@
{
Copy link
Contributor

@EdCaunt EdCaunt Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #11.    ax.set_title("Off the grid sparse positions")

"Off-grid sparse positions"


Reply via ReviewNB

@@ -0,0 +1,965 @@
{
Copy link
Contributor

@EdCaunt EdCaunt Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #3.    ax.imshow(u.data[1], vmin=0, vmax=2, cmap="jet", extent=[0,1,0,1])

A colourbar is needed here


Reply via ReviewNB

Copy link
Contributor

@EdCaunt EdCaunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this now

@@ -0,0 +1,889 @@
{
Copy link
Contributor

@georgebisbas georgebisbas Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how helpful this figure is.

Could be better probably with less points, to see understand weights better


Reply via ReviewNB

@@ -0,0 +1,889 @@
{
Copy link
Contributor

@georgebisbas georgebisbas Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that it is larger, it makes more sense..


Reply via ReviewNB

@mloubout mloubout merged commit cb24675 into master Apr 4, 2024
31 checks passed
@mloubout mloubout deleted the notebook-fix branch April 4, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants